Add integrity checks during package upload#71
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces integrity checking for package uploads by implementing SHA256 checksums for both individual files and overall package content. The changes enable better error detection and debugging by allowing the server to verify package integrity and identify potential corruption issues.
- Adds SHA256 checksum calculation for individual files within packages
- Implements overall package integrity verification by hashing all file checksums
- Includes checksum metadata in package manifests and upload headers
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/tower-package/Cargo.toml | Adds sha2 dependency for cryptographic hashing |
| crates/tower-package/src/lib.rs | Implements core checksum functionality and adds checksum field to manifest |
| crates/tower-package/tests/package_test.rs | Adds test assertion to verify checksum is populated in manifest |
| crates/tower-cmd/src/util/deploy.rs | Computes package checksum and includes it in upload request header |
| crates/tower-cmd/src/util/progress.rs | Minor formatting changes with whitespace additions |
Comments suppressed due to low confidence (1)
crates/tower-package/tests/package_test.rs:204
- The test only checks that a checksum exists but doesn't verify its correctness or format. Consider adding a test that validates the checksum is a valid SHA256 hex string (64 characters, hexadecimal).
assert!(!manifest.checksum.is_empty(), "Manifest integrity check was not set");
|
|
||
| for key in sorted_keys { | ||
| // We need to sort the keys so that we can compute a consistent hash. | ||
| let value = path_hashes.get(key).unwrap(); |
There was a problem hiding this comment.
Using unwrap() is unsafe here. Since we're iterating over keys from the same HashMap, this should never fail, but it's better to handle the error case properly or use a more explicit approach like path_hashes[key].
| let value = path_hashes.get(key).unwrap(); | |
| let value = path_hashes.get(key).expect("Key not found in path_hashes"); |
| Ok(format!("{:x}", result)) | ||
| } | ||
|
|
||
| pub async fn compute_sha256_file(file_path: &PathBuf) -> Result<String, Error> { |
There was a problem hiding this comment.
[nitpick] This function is marked as pub but appears to be an internal utility. Consider making it private or moving it to a separate module if it needs to be shared between crates.
| pub async fn compute_sha256_file(file_path: &PathBuf) -> Result<String, Error> { | |
| async fn compute_sha256_file(file_path: &PathBuf) -> Result<String, Error> { |
| Ok(hash) => hash, | ||
| Err(e) => { | ||
| debug!("Failed to compute package hash: {}", e); | ||
| output::die("Tower CLI failed to properly prepare your package for deployment. Check that you have permissions to read/write to your temporary directory, and if it keeps happening contact Tower support at https://tower.dev"); |
There was a problem hiding this comment.
The error message is generic and doesn't mention the specific failure (checksum computation). Consider making it more specific: 'Tower CLI failed to compute package checksum during deployment preparation.'
| output::die("Tower CLI failed to properly prepare your package for deployment. Check that you have permissions to read/write to your temporary directory, and if it keeps happening contact Tower support at https://tower.dev"); | |
| output::die("Tower CLI failed to compute package checksum during deployment preparation. Check that you have permissions to read/write to your temporary directory, and if it keeps happening contact Tower support at https://tower.dev"); |
| } | ||
|
|
||
| fn compute_sha256_package(path_hashes: &HashMap<PathBuf, String>) -> Result<String, Error> { | ||
| let mut sorted_keys: Vec<&PathBuf> = path_hashes.keys().collect(); |
There was a problem hiding this comment.
maybe dumb question, but it seems we're passing in a full (relative?) path, whereas in the go code we're hashing the filename — are these the same?
There was a problem hiding this comment.
I think their cooincidentally the same. I need to do more research. This is especially interesting in the case of e.g. Windows. Let's ship this, and I'll follow up with a bug fix accordingly.
socksy
left a comment
socksy
left a comment
There was a problem hiding this comment.
Other than filepath question, looks good 👍
This PR introduces calculating package content integrity checks, as well as overall package integrity checks that are sent to the server. This allows us to figure out what's going on with clients on the server, and return better error messages about potential corruption.